Emit warning for invalid use of quoted types in union syntax #18183
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Conformance results diff for this PR (improves conformance in :...skipping...
diff --git a/conformance/results/mypy/annotations_forward_refs.toml b/conformance/results/mypy/annotations_forward_refs.toml
index bee7ebb..6763a18 100644
--- a/conformance/results/mypy/annotations_forward_refs.toml
+++ b/conformance/results/mypy/annotations_forward_refs.toml
@@ -5,6 +5,8 @@ Does not report error for use of quoted type with "|" operator (runtime error).
Incorrectly generates error for quoted type defined in class scope.
"""
output = """
+annotations_forward_refs.py:24: error: X | Y syntax for unions cannot use quoted operands; use quotes around the entire expression instead [misc]
+annotations_forward_refs.py:25: error: X | Y syntax for unions cannot use quoted operands; use quotes around the entire expression instead [misc]
annotations_forward_refs.py:41: error: Invalid type comment or annotation [valid-type]
annotations_forward_refs.py:42: error: Invalid type comment or annotation [valid-type]
annotations_forward_refs.py:43: error: Invalid type comment or annotation [valid-type]
@@ -33,8 +35,6 @@ conformance_automated = "Fail"
errors_diff = """
Line 22: Expected 1 errors
Line 23: Expected 1 errors
-Line 24: Expected 1 errors
-Line 25: Expected 1 errors
Line 66: Expected 1 errors
Line 87: Unexpected errors ['annotations_forward_refs.py:87: error: Function "annotations_forward_refs.ClassD.int" is not valid as a type [valid-type]']
Line 96: Unexpected errors ['annotations_forward_refs.py:96: error: Expression is of type int?, not "int" [assert-type]']
diff --git a/conformance/results/mypy/version.toml b/conformance/results/mypy/version.toml
index 51d97b4..b1035e1 100644
--- a/conformance/results/mypy/version.toml
+++ b/conformance/results/mypy/version.toml
@@ -1,2 +1,2 @@
-version = "mypy 1.14.0+dev.499adaed8adbded1a180e30d071438fef81779ec"
-test_duration = 2.4
+version = "mypy 1.14.0+dev.ce34db689d8a6cab6f540a2c38ecc32952534b0a"
+test_duration = 4.6 |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
JukkaL
reviewed
Nov 27, 2024
|
|
||
| T = TypeVar("T") | ||
|
|
||
| ok1: TypeAlias = T | "int" |
There was a problem hiding this comment.
Would it be useful to have more tests for things like "int | str" | T or "int | str", here and elsewhere?
| t.original_str_expr is None | ||
| and not self.api.is_stub_file | ||
| and (not self.api.is_future_flag_set("annotations") or self.defining_alias) | ||
| ): |
There was a problem hiding this comment.
Style nit: This new if statement now accounts for most the code in this function. What about moving it to a helper function to make the basic logic clearer?
| x3: int | str | "bytes" # E: X | Y syntax for unions cannot use quoted operands; use quotes around the entire expression instead | ||
| assert_type(x1, Union[int, str]) | ||
| assert_type(x2, Union[int, str]) | ||
| assert_type(x3, Union[int, str, bytes]) |
There was a problem hiding this comment.
Test also valid cases, like Callable[[], None] | "int", which work at runtime? Or are these tested in existing tests?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Using quoted types in PEP 604 union syntax can be problematic, since at runtime, using
|between a type and string produces an error:The exception is when the type is a
TypeVaror a_UnionGenericAlias, which can both be safely|'d with strings:This PR makes mypy emit a warning for the former cases (where using
|with a string will produce a runtime error) while permitting the latter cases (where using|with a string is safe).A few notes about the implementation:
from __future__ import annotations. Like in stubs, quoted types in these cases are redundant but mostly harmless (aside from issues with runtime inspection). It might be a good idea to emit errors for these cases too (pyright does), but I wasn't sure if the benefit is enough to justify the churn. Without this exception, this PR has ~20 mypy_primer hits across 4 projects.